Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to the ES model to allow the configuration of an ES-import route target #1109

Merged

Conversation

abamberger-arista
Copy link
Contributor

Change Scope

The existing openconfig ethernet segment model has no way to configure the ES-import route target (defined in RFC7432 Section 7.6) for a type 0 operator configured ESI. This proposal adds a new field, “es-import-route-target”, to the existing ES config model, to allow the explicit configuration of an ES-import route target.

Because this is a new leaf being added, this change is fully backwards compatible.

New tree state after proposed change (additions in bold):

module: openconfig-ethernet-segments
+--rw ethernet-segments
   +--rw ethernet-segment* [name]
      +--rw config
      |  +--rw es-import-route-target?   oc-yang-types:mac-address
      |  ...
      +--ro state
         +--ro es-import-route-target?   oc-yang-types:mac-address
	   ...

New Yang Paths:

  • ethernet-segments/ethernet-segment/config/es-import-route-target
  • ethernet-segments/ethernet-segment/state/es-import-route-target

Platform Implementations

Arista EOS:

https://www.arista.com/en/um-eos/eos-vxlan-configuration#topic_ckc_dh4_ynb
https://www.arista.com/en/support/toi/eos-4-22-0f/14236-evpn-vxlan-all-active-multihoming

interface Port-Channel1
   evpn ethernet-segment
      identifier 00aa:bbbb:cccc:dddd:eeee
      route-target import 12:23:34:45:56:67

…ute target

* Add a new leaf "es-import-route-target" to the Ethernet Segment config
  model to allow the explicit configuration of an ES-import route target
  as per RFC7432 Section 7.6
@abamberger-arista abamberger-arista requested a review from a team as a code owner May 8, 2024 22:02
@jorabada
Copy link

jorabada commented May 9, 2024

The ES import route target is autoderived in case of ES types 1, 2 and 3, and may be autoderived in the rest of the cases too. That is what other implementations do as well. So I don't think this change is necessary...

@abamberger-arista
Copy link
Contributor Author

RFC7432 only specifies how the ES-import RT can be auto-derived from ES types 1, 2, and 3. In particular, for type 0 ESs, since they're operator configured and completely arbitrary, and the RFC doesn't specify any way to do auto-derivation for that type, I think this is required at least to be able to properly support type 0 ESs in a general way.

…ute target

* Add a new leaf "es-import-route-target" to the Ethernet Segment config
  model to allow the explicit configuration of an ES-import route target
  as per RFC7432 Section 7.6
* Make this new "es-import-route-target" leaf conditional on the type of
  ESI being Type 0 (operator configured), as types 1, 2, and 3 can be
  auto-derived from the ESI (it's possible for types 4 and 5 to be as
  well, but that's not called out in the RFC, so leave those alone for
  now, they can be added to the when statement in the future if
  necessary)
…ute target

* Add a new leaf "es-import-route-target" to the Ethernet Segment config
  model to allow the explicit configuration of an ES-import route target
  as per RFC7432 Section 7.6
* Make this new "es-import-route-target" leaf conditional on the type of
  ESI being Type 0 (operator configured), as types 1, 2, and 3 can be
  auto-derived from the ESI (it's possible for types 4 and 5 to be as
  well, but that's not called out in the RFC, so leave those alone for
  now, they can be added to the when statement in the future if
  necessary)
@abamberger-arista
Copy link
Contributor Author

I updated the PR to add a when clause to the new "es-import-route-target" leaf to make it only apply when the ESI type is Type-0, which should make this a bit cleaner to use. As mentioned earlier, types 1, 2, and 3 are defined by the RFC to be auto-derived from the ESI, so don't require an explicitly specified es-import-rt. Types 4 and 5 aren't specified one way or the other, but the when clause can be updated to include these in the future if necessary.

@dplore
Copy link
Member

dplore commented Aug 6, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Aug 6, 2024

No major YANG version changes in commit 3f64e22

@dplore
Copy link
Member

dplore commented Aug 6, 2024

@jorabada can you please comment?

@dplore dplore self-assigned this Aug 6, 2024
@jorabada
Copy link

I updated the PR to add a when clause to the new "es-import-route-target" leaf to make it only apply when the ESI type is Type-0, which should make this a bit cleaner to use. As mentioned earlier, types 1, 2, and 3 are defined by the RFC to be auto-derived from the ESI, so don't require an explicitly specified es-import-rt. Types 4 and 5 aren't specified one way or the other, but the when clause can be updated to include these in the future if necessary.

I don't think I agree with this. The es-import route-target can be autoderived for all types. rfc7432bis states that is autoderived for types 1..3, and MAY be autoderived for the other types. If you do this for type 0, I don't see why you wouldn't do it for the other types. Also, I suppose implementations do autoderivation as a bare minimum, and explicit configuration is supported only in some implementations out there.

@abamberger-arista
Copy link
Contributor Author

Would it make more sense to change the "es-import-route-target" leaf to be a union of either an explicit value, or an enum with an AUTO member? That would then allow the user to be more explicit, both when configuring AUTO, and when configuring an explicit es-import-rt. The explicit configuration portion of the union could be made conditional on types 0, 4, and 5, to adhere to the new draft RFC.

@jorabada
Copy link

Thanks @abamberger-arista - as I was saying, I don't think this change is necessary at all. If there is really push from people to have it, your original suggestion is simpler: When configured explicitly, the es-import-route-target will be used. Otherwise, the autoderived one is used.

@abamberger-arista
Copy link
Contributor Author

@jorabada I guess I'm not seeing a good reason not to add it. It's something explicitly defined in the RFC, it's backwards compatible, and it's optional. I can revert back to the original proposal (which I agree is simpler, although it would technically allow users to push an explicit es-import-rt with ESI types that per the RFC are always supposed to be auto-generated, that is, types 1-3). Are there any objections to getting this in in its original form (a single additional optional es-import-rt leaf)?

@jorabada
Copy link

jorabada commented Sep 6, 2024

@jorabada I guess I'm not seeing a good reason not to add it. It's something explicitly defined in the RFC, it's backwards compatible, and it's optional. I can revert back to the original proposal (which I agree is simpler, although it would technically allow users to push an explicit es-import-rt with ESI types that per the RFC are always supposed to be auto-generated, that is, types 1-3). Are there any objections to getting this in in its original form (a single additional optional es-import-rt leaf)?

As I said, I don't think it is necessary, but I won't object if you add it (in its original form)... thanks.

* Add a new leaf "es-import-route-target" to the Ethernet Segment config
  model to allow the explicit configuration of an ES-import route target
  as per RFC7432 Section 7.6
…r-arista/openconfig-public into ethernet-segment-model-updates
@abamberger-arista
Copy link
Contributor Author

Thanks for taking a look again, I've restored it to its original form. I understand this may be a niche use case in practice, but since I don't really see much of a downside in adding it, and it will allow implementations that do support it to be a bit more full featured when using OC for configuration. @dplore are all concerns resolved from your end on this?

@abamberger-arista
Copy link
Contributor Author

/gcbrun

@dplore
Copy link
Member

dplore commented Sep 11, 2024

/gcbrun

@dplore dplore added the last-call PR that is in final review before merging. label Oct 7, 2024
@dplore
Copy link
Member

dplore commented Oct 7, 2024

/gcbrun

Comment on lines 212 to 213
"Received ES routes with an ES-Import extended community set to this value
are imported into this ethernet segment";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add text regarding auto derived and what happens if configured? ie:

"When configured by a user, the es-import-route-target will be used, otherwise the system automatically derives the value used."
Also, end the sentence with a period please. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting to last call for comments. THis will merge Oct 21, 2024

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dplore
Copy link
Member

dplore commented Oct 22, 2024

/gcbrun

@dplore
Copy link
Member

dplore commented Oct 22, 2024

/gcbrun

@dplore dplore merged commit 881b9e6 into openconfig:master Oct 22, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call PR that is in final review before merging. non-breaking
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants